Use `Build::getenv` instead of `env::var*` in anywhere that makes sense #1103

NobodyXu · 2024-06-23T15:13:02Z

So that we would get:

recompilation when the env is changed
cached env

Also some optimisation

Only call `self.getenv("RUSTC_LINKER")` if necessary Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

`RwLock` would enable readers to run in parallel. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Use `Path` instead of `PathBuf` to avoid alloc Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Cache everything in there in `apple_versions_cache` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Cache in `apple_sdk_root_cache` if possible Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Only call `to_string_lossy()` if necessary Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

src/lib.rs

Avoid one heap allocation that is de-allocated immediately

src/lib.rs

madsmtm

Might make sense to ban std::env::var and std::env::var_os via. Clippy's disallowed_methods functionality? And then explicitly allow it inside the implementation of getenv

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

under `#[cfg(test)]` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2024-06-24T14:50:18Z

Might make sense to ban std::env::var and std::env::var_os via. Clippy's disallowed_methods functionality? And then explicitly allow it inside the implementation of getenv

Thanks!

Applied the suggestion, and turns out that the entire windows_registry is using env::var*

Took me a while to change them to use a generic function.

cc @thomcc @madsmtm

NobodyXu · 2024-06-24T14:58:05Z

cc @madsmtm This PR should cover most env variables in #906

If there's any missing, feel free to open a comment.

The generic params would cause every function to be mono-ed twice, result in longer compilation time and larger binary. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Create a new local fn `has_msbuild_version` in `find_vs_version` to make the invocation easier. Also add `#[inline(always)]` to `has_msbuild_version` since it is only used in `find_vs_version` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2024-06-25T14:08:38Z

Updated the code to remove any generics in windows_regsitry.

Having generics would make cc takes longer to compile and generate larger binary, and I don't think it would give us any improvements to performance.

oconnor663 · 2024-07-12T01:00:26Z

src/lib.rs

@@ -3534,7 +3540,7 @@ impl Build {
        // Loop through PATH entries searching for each toolchain. This ensures that we
        // are more likely to discover the toolchain early on, because chances are good
        // that the desired toolchain is in one of the higher-priority paths.
-        env::var_os("PATH")


It looks like this change causes crates to rebuild on Windows any time the PATH changes. Anecdotally, that seems to have a pretty high false positive rate, and since some PATH changes are associated with tools like rust-analyer that run constantly in the background, this can make it so that every build is a complete rebuild in some setups / with some dependencies. See for example BLAKE3-team/BLAKE3#324. I might file this as an issue shortly, but I wanted to mention it real quick before I forget.

Perhaps we can special-case PATH here?

For example, Update the getenv to not emit anything for PATH

Just crosslinking, should be fixed in #1215

NobodyXu added 9 commits June 23, 2024 23:09

Optimize prefix_for_target

3138001

Only call `self.getenv("RUSTC_LINKER")` if necessary Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Refactor envflags

8a7ccdd

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Optimize perf: Use RwLock instead of Mutex

90fae8e

`RwLock` would enable readers to run in parallel. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Use self.getenv for SDKROOT and in apple_deployment_version()

b1a3027

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Optimize apple_sdk_root`

905a7e3

Use `Path` instead of `PathBuf` to avoid alloc Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Optimize apple_deployment_version

bad5c9c

Cache everything in there in `apple_versions_cache` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Optimize apple_sdk_root

918a216

Cache in `apple_sdk_root_cache` if possible Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Refactor and optimize apple_sdk_root_inner

a90e3be

Only call `to_string_lossy()` if necessary Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Use Build::getenv in rustc_wrapper_fallback

a48587b

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu requested review from thomcc and ChrisDenton June 23, 2024 15:16

NobodyXu commented Jun 24, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Optimize src/lib.rs

bdf29f5

Avoid one heap allocation that is de-allocated immediately

NobodyXu commented Jun 24, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Fix cargo fmt i src/lib.rs

6e485a7

madsmtm approved these changes Jun 24, 2024

View reviewed changes

Disallow std::env::var(_os) using clippy lints

8d3c02a

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

thomcc approved these changes Jun 24, 2024

View reviewed changes

NobodyXu added 5 commits June 25, 2024 00:32

Update windows::find_tools to use geneirc get_env

f122d2d

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Use Build::getenv for windows_registry::find used in Build

765dc58

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Optimize: Put test_android_clang_compiler_uses_target_arg_internally

94733ca

under `#[cfg(test)]` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

FIx clippy lints

14c8382

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Rm clippy disallowed-methods rules that do not work

f3aec09

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu requested review from thomcc and madsmtm June 24, 2024 14:49

NobodyXu added 3 commits June 25, 2024 23:31

Optimize windows_registry: Rm generic parameters

c8a3476

The generic params would cause every function to be mono-ed twice, result in longer compilation time and larger binary. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Put #[inline(always)] on very simple/trivial function

a468aaa

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Remove generics in windows_registry

db71e85

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Refactor and optimize find_vs_version

8fe16d4

Create a new local fn `has_msbuild_version` in `find_vs_version` to make the invocation easier. Also add `#[inline(always)]` to `has_msbuild_version` since it is only used in `find_vs_version` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu force-pushed the optimized branch from cb42a06 to 8fe16d4 Compare June 25, 2024 13:43

Optimize get_sdk10_dir: Use From<Env> for PathBuf

d2df335

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

thomcc approved these changes Jun 25, 2024

View reviewed changes

NobodyXu merged commit 4cb5b94 into main Jun 25, 2024
48 checks passed

NobodyXu deleted the optimized branch June 25, 2024 22:42

github-actions bot mentioned this pull request Jun 25, 2024

chore: release v1.0.101 #1106

Merged

NobodyXu mentioned this pull request Jun 25, 2024

Rebuild after modifying environment variables that the C compiler reads #906

Open

This was referenced Jul 11, 2024

Continual recompliation of blake3 BLAKE3-team/BLAKE3#324

Closed

Do not let cc-rs trigger a rebuild any time the env changes BLAKE3-team/BLAKE3#413

Merged

oconnor663 reviewed Jul 12, 2024

View reviewed changes

nmattia mentioned this pull request Nov 7, 2024

Use relative directory for obj files hash #1270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `Build::getenv` instead of `env::var*` in anywhere that makes sense #1103

Use `Build::getenv` instead of `env::var*` in anywhere that makes sense #1103

NobodyXu commented Jun 23, 2024

madsmtm left a comment

NobodyXu commented Jun 24, 2024

NobodyXu commented Jun 24, 2024

NobodyXu commented Jun 25, 2024

oconnor663 Jul 12, 2024

NobodyXu Jul 12, 2024

tgross35 Sep 27, 2024

Use Build::getenv instead of env::var* in anywhere that makes sense #1103

Use Build::getenv instead of env::var* in anywhere that makes sense #1103

Conversation

NobodyXu commented Jun 23, 2024

madsmtm left a comment

Choose a reason for hiding this comment

NobodyXu commented Jun 24, 2024

NobodyXu commented Jun 24, 2024

NobodyXu commented Jun 25, 2024

oconnor663 Jul 12, 2024

Choose a reason for hiding this comment

NobodyXu Jul 12, 2024

Choose a reason for hiding this comment

tgross35 Sep 27, 2024

Choose a reason for hiding this comment

Use `Build::getenv` instead of `env::var*` in anywhere that makes sense #1103

Use `Build::getenv` instead of `env::var*` in anywhere that makes sense #1103